Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ngcc): consistently delegate to TypeScript host for typing files #36089

Closed
wants to merge 3 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Mar 16, 2020

When ngcc is compiling an entry-point, it uses a ReflectionHost that
is specific to its format, e.g. ES2015, ES5, UMD or CommonJS. During the
compilation of that entry-point however, the reflector may be used to
reflect into external libraries using their declaration files.

Up until now this was achieved by letting all ReflectionHost classes
consider their parent class for reflector queries, thereby ending up in
the TypeScriptReflectorHost that is a common base class for all
reflector hosts. This approach has proven to be prone to bugs, as
failing to call into the base class would cause incompatibilities with
reading from declaration files.

The observation can be made that there's only two distinct kinds of
reflection host queries:

  1. the reflector query is about code that is part of the entry-point
    that is being compiled, or
  2. the reflector query is for an external library that the entry-point
    depends on, in which case the information is reflected
    from the declaration files.

The ReflectionHost that was chosen for the entry-point should serve
only reflector queries for the first case, whereas a regular
TypeScriptReflectionHost should be used for the second case. This
avoids avoids the problem where a format-specific ReflectionHost fails
to handle the second case correctly, as it isn't even considered for
such reflector queries.

This commit introduces a ReflectionHost that delegates to the
TypeScriptReflectionHost for AST nodes within declaration files,
otherwise delegating to the format-specific ReflectionHost.

Fixes #35078
Resolves FW-1859

@ngbot ngbot bot added this to the needsTriage milestone Mar 16, 2020
@JoostK JoostK marked this pull request as ready for review March 16, 2020 19:54
@JoostK JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 16, 2020
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff @JoostK !
Did you do any performance benchmarks? I don't expect it to be a big difference since the majority of the cost is in creating the original TS program, and you have removed some complexity from the individual ngcc reflectionhosts, but it would be interesting to see...

@JoostK
Copy link
Member Author

JoostK commented Mar 16, 2020

@petebacondarwin I have not, but I'm not expecting it to become slower. Reflected nodes are typically not deep in the AST so asking for the source file should be relatively few parent pointer reads.

JoostK added a commit to JoostK/ngcc-validation that referenced this pull request Mar 16, 2020
@JoostK
Copy link
Member Author

JoostK commented Mar 16, 2020

It's green on ngcc-validation: angular/ngcc-validation#1015

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍 ✨ 👌
(Just a couple minor questions/suggestions.)

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 17, 2020
@gkalpak
Copy link
Member

gkalpak commented Mar 17, 2020

Nit: There is also a typo in the first commit message: avoids avoids --> avoids

@JoostK JoostK force-pushed the ngcc-delegating-host branch 2 times, most recently from 5d07978 to 13d8f79 Compare March 17, 2020 19:29
The format property for ES5 bundles should be "module" or "es5"/"esm5",
but was "main" instead. The "main" property is appropriate for CommonJS
and UMD bundles, not for ES5 bundles.
When ngcc is compiling an entry-point, it uses a `ReflectionHost` that
is specific to its format, e.g. ES2015, ES5, UMD or CommonJS. During the
compilation of that entry-point however, the reflector may be used to
reflect into external libraries using their declaration files.

Up until now this was achieved by letting all `ReflectionHost` classes
consider their parent class for reflector queries, thereby ending up in
the `TypeScriptReflectionHost` that is a common base class for all
reflector hosts. This approach has proven to be prone to bugs, as
failing to call into the base class would cause incompatibilities with
reading from declaration files.

The observation can be made that there's only two distinct kinds of
reflection host queries:
1. the reflector query is about code that is part of the entry-point
   that is being compiled, or
2. the reflector query is for an external library that the entry-point
   depends on, in which case the information is reflected
   from the declaration files.

The `ReflectionHost` that was chosen for the entry-point should serve
only reflector queries for the first case, whereas a regular
`TypeScriptReflectionHost` should be used for the second case. This
avoids the problem where a format-specific `ReflectionHost` fails to
handle the second case correctly, as it isn't even considered for such
reflector queries.

This commit introduces a `ReflectionHost` that delegates to the
`TypeScriptReflectionHost` for AST nodes within declaration files,
otherwise delegating to the format-specific `ReflectionHost`.

Fixes angular#35078
Resolves FW-1859
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 17, 2020
AndrewKushnir pushed a commit that referenced this pull request Mar 17, 2020
…36089)

When ngcc is compiling an entry-point, it uses a `ReflectionHost` that
is specific to its format, e.g. ES2015, ES5, UMD or CommonJS. During the
compilation of that entry-point however, the reflector may be used to
reflect into external libraries using their declaration files.

Up until now this was achieved by letting all `ReflectionHost` classes
consider their parent class for reflector queries, thereby ending up in
the `TypeScriptReflectionHost` that is a common base class for all
reflector hosts. This approach has proven to be prone to bugs, as
failing to call into the base class would cause incompatibilities with
reading from declaration files.

The observation can be made that there's only two distinct kinds of
reflection host queries:
1. the reflector query is about code that is part of the entry-point
   that is being compiled, or
2. the reflector query is for an external library that the entry-point
   depends on, in which case the information is reflected
   from the declaration files.

The `ReflectionHost` that was chosen for the entry-point should serve
only reflector queries for the first case, whereas a regular
`TypeScriptReflectionHost` should be used for the second case. This
avoids the problem where a format-specific `ReflectionHost` fails to
handle the second case correctly, as it isn't even considered for such
reflector queries.

This commit introduces a `ReflectionHost` that delegates to the
`TypeScriptReflectionHost` for AST nodes within declaration files,
otherwise delegating to the format-specific `ReflectionHost`.

Fixes #35078
Resolves FW-1859

PR Close #36089
AndrewKushnir pushed a commit that referenced this pull request Mar 17, 2020
The format property for ES5 bundles should be "module" or "es5"/"esm5",
but was "main" instead. The "main" property is appropriate for CommonJS
and UMD bundles, not for ES5 bundles.

PR Close #36089
AndrewKushnir pushed a commit that referenced this pull request Mar 17, 2020
…36089)

When ngcc is compiling an entry-point, it uses a `ReflectionHost` that
is specific to its format, e.g. ES2015, ES5, UMD or CommonJS. During the
compilation of that entry-point however, the reflector may be used to
reflect into external libraries using their declaration files.

Up until now this was achieved by letting all `ReflectionHost` classes
consider their parent class for reflector queries, thereby ending up in
the `TypeScriptReflectionHost` that is a common base class for all
reflector hosts. This approach has proven to be prone to bugs, as
failing to call into the base class would cause incompatibilities with
reading from declaration files.

The observation can be made that there's only two distinct kinds of
reflection host queries:
1. the reflector query is about code that is part of the entry-point
   that is being compiled, or
2. the reflector query is for an external library that the entry-point
   depends on, in which case the information is reflected
   from the declaration files.

The `ReflectionHost` that was chosen for the entry-point should serve
only reflector queries for the first case, whereas a regular
`TypeScriptReflectionHost` should be used for the second case. This
avoids the problem where a format-specific `ReflectionHost` fails to
handle the second case correctly, as it isn't even considered for such
reflector queries.

This commit introduces a `ReflectionHost` that delegates to the
`TypeScriptReflectionHost` for AST nodes within declaration files,
otherwise delegating to the format-specific `ReflectionHost`.

Fixes #35078
Resolves FW-1859

PR Close #36089
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 2, 2020
The `NgccReflectionHost`'s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to evaluate otherwise.

This commit moves the logic for identifying these known declarations to
dedicated methods. This is in preparation of allowing ngcc's
`DelegatingReflectionHost` (introduced in angular#36089) to also apply the
known declaration detection logic when reflecting on TypeScript sources.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 2, 2020
…nHost`s

In angular#36089, `DelegatingReflectionHost` was introduced. Under the hood, it
delegates another `NgccReflectionHost` in order to reflect over the
program's source files, while using a different TypeScript
`ReflectionHost` to reflect over `.d.ts` files (which is how external
dependencies are represented in the program).

Previously, the `NgccReflectionHost`s were used directly in tests. This
does not exercise them in the way they are exercised in the actual
program, because (when used directly) they will also reflect on `.d.ts`
files too (instead of delegating to the TypeScript `ReflectionHost`.
This could hide bugs that would happen on the actual program.

This commit fixes this by using the `DelegatingReflectionHost` in the
various `NgccReflectionHost` tests.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 2, 2020
The `NgccReflectionHost`'s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to evaluate otherwise.

In angular#36089, `DelegatingReflectionHost` was introduced, which delegates to
a TypeScript `ReflectionptHost` when reflecting on TypeScript files,
which for ngcc's case means `.d.ts` files of dependencies. As a result,
ngcc lost the ability to detect imported TypeScript helpers from
`tslib`, because `DelegatingReflectionHost` was not able to apply the
known declaration detection logic while reflecting on `tslib`'s `.d.ts`
files.

This commit fixes this by ensuring `DelegatingReflectionHost` calls the
`NgccReflectionHost`'s `addKnownDeclaration()` method as necessary, even
when using the TypeScript `ReflectionHost`.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`'s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to evaluate otherwise.

This commit moves the logic for identifying these known declarations to
dedicated methods. This is in preparation of allowing ngcc's
`DelegatingReflectionHost` (introduced in angular#36089) to also apply the
known declaration detection logic when reflecting on TypeScript sources.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
…nHost`s

In angular#36089, `DelegatingReflectionHost` was introduced. Under the hood, it
delegates another `NgccReflectionHost` in order to reflect over the
program's source files, while using a different TypeScript
`ReflectionHost` to reflect over `.d.ts` files (which is how external
dependencies are represented in the program).

Previously, the `NgccReflectionHost`s were used directly in tests. This
does not exercise them in the way they are exercised in the actual
program, because (when used directly) they will also reflect on `.d.ts`
files too (instead of delegating to the TypeScript `ReflectionHost`.
This could hide bugs that would happen on the actual program.

This commit fixes this by using the `DelegatingReflectionHost` in the
various `NgccReflectionHost` tests.

NOTE:
This change will cause some of the existing tests to start failing.
These failures demonstrate pre-existing bugs in ngcc, that were hidden
due to the tests' being inconsistent with how the `ReflectionHost`s are
used in the actual program. They will be fixed in the next commit.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`'s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to evaluate otherwise.

In angular#36089, `DelegatingReflectionHost` was introduced, which delegates to
a TypeScript `ReflectionptHost` when reflecting on TypeScript files,
which for ngcc's case means `.d.ts` files of dependencies. As a result,
ngcc lost the ability to detect imported TypeScript helpers from
`tslib`, because `DelegatingReflectionHost` was not able to apply the
known declaration detection logic while reflecting on `tslib`'s `.d.ts`
files.

This commit fixes this by ensuring `DelegatingReflectionHost` calls the
`NgccReflectionHost`'s `addKnownDeclaration()` method as necessary, even
when using the TypeScript `ReflectionHost`.

NOTE:
The previous commit exposed a bug in ngcc that was hidden due to the
tests' being inconsistent with how the `ReflectionHost`s are used in the
actual program. The changes in the commit are verified by ensuring the
failing tests are now passing (hence no new tests are added).
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

This commit moves the logic for identifying these known declarations to
dedicated methods. This is in preparation of allowing ngcc's
`DelegatingReflectionHost` (introduced in angular#36089) to also apply the
known declaration detection logic when reflecting on TypeScript sources.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
…nHost`s

In angular#36089, `DelegatingReflectionHost` was introduced. Under the hood, it
delegates another `NgccReflectionHost` in order to reflect over the
program's source files, while using a different TypeScript
`ReflectionHost` to reflect over `.d.ts` files (which is how external
dependencies are represented in the program).

Previously, the `NgccReflectionHost`s were used directly in tests. This
does not exercise them in the way they are exercised in the actual
program, because (when used directly) they will also reflect on `.d.ts`
files too (instead of delegating to the TypeScript `ReflectionHost`).
This could hide bugs that would happen on the actual program.

This commit fixes this by using the `DelegatingReflectionHost` in the
various `NgccReflectionHost` tests.

NOTE:
This change will cause some of the existing tests to start failing.
These failures demonstrate pre-existing bugs in ngcc, that were hidden
due to the tests' being inconsistent with how the `ReflectionHost`s are
used in the actual program. They will be fixed in the next commit.
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

In angular#36089, `DelegatingReflectionHost` was introduced, which delegates to
a TypeScript `ReflectionHost` when reflecting on TypeScript files, which
for ngcc's case means `.d.ts` files of dependencies. As a result, ngcc
lost the ability to detect TypeScript helpers imported from `tslib`,
because `DelegatingReflectionHost` was not able to apply the known
declaration detection logic while reflecting on `tslib`'s `.d.ts` files.

This commit fixes this by ensuring `DelegatingReflectionHost` calls the
`NgccReflectionHost`'s `detectKnownDeclaration()` method as necessary,
even when using the TypeScript `ReflectionHost`.

NOTE:
The previous commit exposed a bug in ngcc that was hidden due to the
tests' being inconsistent with how the `ReflectionHost`s are used in the
actual program. The changes in this commit are verified by ensuring the
failing tests are now passing (hence no new tests are added).
kara pushed a commit that referenced this pull request Apr 3, 2020
…od (#36284)

The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

This commit moves the logic for identifying these known declarations to
dedicated methods. This is in preparation of allowing ngcc's
`DelegatingReflectionHost` (introduced in #36089) to also apply the
known declaration detection logic when reflecting on TypeScript sources.

PR Close #36284
kara pushed a commit that referenced this pull request Apr 3, 2020
…nHost`s (#36284)

In #36089, `DelegatingReflectionHost` was introduced. Under the hood, it
delegates another `NgccReflectionHost` in order to reflect over the
program's source files, while using a different TypeScript
`ReflectionHost` to reflect over `.d.ts` files (which is how external
dependencies are represented in the program).

Previously, the `NgccReflectionHost`s were used directly in tests. This
does not exercise them in the way they are exercised in the actual
program, because (when used directly) they will also reflect on `.d.ts`
files too (instead of delegating to the TypeScript `ReflectionHost`).
This could hide bugs that would happen on the actual program.

This commit fixes this by using the `DelegatingReflectionHost` in the
various `NgccReflectionHost` tests.

NOTE:
This change will cause some of the existing tests to start failing.
These failures demonstrate pre-existing bugs in ngcc, that were hidden
due to the tests' being inconsistent with how the `ReflectionHost`s are
used in the actual program. They will be fixed in the next commit.

PR Close #36284
kara pushed a commit that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

In #36089, `DelegatingReflectionHost` was introduced, which delegates to
a TypeScript `ReflectionHost` when reflecting on TypeScript files, which
for ngcc's case means `.d.ts` files of dependencies. As a result, ngcc
lost the ability to detect TypeScript helpers imported from `tslib`,
because `DelegatingReflectionHost` was not able to apply the known
declaration detection logic while reflecting on `tslib`'s `.d.ts` files.

This commit fixes this by ensuring `DelegatingReflectionHost` calls the
`NgccReflectionHost`'s `detectKnownDeclaration()` method as necessary,
even when using the TypeScript `ReflectionHost`.

NOTE:
The previous commit exposed a bug in ngcc that was hidden due to the
tests' being inconsistent with how the `ReflectionHost`s are used in the
actual program. The changes in this commit are verified by ensuring the
failing tests are now passing (hence no new tests are added).

PR Close #36284
kara pushed a commit that referenced this pull request Apr 3, 2020
…od (#36284)

The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

This commit moves the logic for identifying these known declarations to
dedicated methods. This is in preparation of allowing ngcc's
`DelegatingReflectionHost` (introduced in #36089) to also apply the
known declaration detection logic when reflecting on TypeScript sources.

PR Close #36284
kara pushed a commit that referenced this pull request Apr 3, 2020
…nHost`s (#36284)

In #36089, `DelegatingReflectionHost` was introduced. Under the hood, it
delegates another `NgccReflectionHost` in order to reflect over the
program's source files, while using a different TypeScript
`ReflectionHost` to reflect over `.d.ts` files (which is how external
dependencies are represented in the program).

Previously, the `NgccReflectionHost`s were used directly in tests. This
does not exercise them in the way they are exercised in the actual
program, because (when used directly) they will also reflect on `.d.ts`
files too (instead of delegating to the TypeScript `ReflectionHost`).
This could hide bugs that would happen on the actual program.

This commit fixes this by using the `DelegatingReflectionHost` in the
various `NgccReflectionHost` tests.

NOTE:
This change will cause some of the existing tests to start failing.
These failures demonstrate pre-existing bugs in ngcc, that were hidden
due to the tests' being inconsistent with how the `ReflectionHost`s are
used in the actual program. They will be fixed in the next commit.

PR Close #36284
kara pushed a commit that referenced this pull request Apr 3, 2020
The `NgccReflectionHost`s have logic for detecting certain known
declarations (such as `Object.assign()` and TypeScript helpers), which
allows the `PartialEvaluator` to evaluate expressions it would not be
able to statically evaluate otherwise.

In #36089, `DelegatingReflectionHost` was introduced, which delegates to
a TypeScript `ReflectionHost` when reflecting on TypeScript files, which
for ngcc's case means `.d.ts` files of dependencies. As a result, ngcc
lost the ability to detect TypeScript helpers imported from `tslib`,
because `DelegatingReflectionHost` was not able to apply the known
declaration detection logic while reflecting on `tslib`'s `.d.ts` files.

This commit fixes this by ensuring `DelegatingReflectionHost` calls the
`NgccReflectionHost`'s `detectKnownDeclaration()` method as necessary,
even when using the TypeScript `ReflectionHost`.

NOTE:
The previous commit exposed a bug in ngcc that was hidden due to the
tests' being inconsistent with how the `ReflectionHost`s are used in the
actual program. The changes in this commit are verified by ensuring the
failing tests are now passing (hence no new tests are added).

PR Close #36284
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes freq1: low risk: medium target: patch This PR is targeted for the next patch release type: bug/fix workaround4: none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ivy] Incompatibility of Esm2015ReflectionHost with libraries compiled with NGCC
4 participants